-
Notifications
You must be signed in to change notification settings - Fork 20
Add support to visually represent addresses and hashes with 'blo' #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
All contributors have signed the CLA ✍️ ✅ |
I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement |
Great! Awesome work! Since Safe doesn't use this pixel for hashes, I'd prefer to remove it. Besides, in theory, this library was created for addresses. I thought maybe there could be a collision, but I tested it and it didn't:
|
ok @josepchetrit12 , Tell me what you like: |
20px it's the way to go, network image and pixel avatar should be the same size and style! |
ok cool! Change is committed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Amazing thanks for your contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the changes. Just one small note: there are conflicts in package-lock.json
with the main branch (a dependency that it is not being used). We should regenerate the lock file to clean up any unused dependencies to maintain the dependency tree clean and consistent.
Signed-off-by: Fernando Rabasco Ledesma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@Archethect Can you sign your commits? so we can merge this PR! thanks |
Verified/Signed my last noop commit. |
We need all the commits signed. @Archethect |
Bumps [next](https://github.com/vercel/next.js) from 14.2.25 to 14.2.26. - [Release notes](https://github.com/vercel/next.js/releases) - [Changelog](https://github.com/vercel/next.js/blob/canary/release.js) - [Commits](vercel/next.js@v14.2.25...v14.2.26) --- updated-dependencies: - dependency-name: next dependency-version: 14.2.26 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ferrabled @josepchetrit12 I rebased and signed all my commits. Let me know if anything else is required. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for this feature 🚀
these commits are still not verified :( |
Afaik you can't verify existing commits. The only options I see are a rebase (which I did, which recommits all previous commits signed. However, because they take the commit history into account, they are new ones) or a totally new PR from scratch discarding all commits in this PR. |
Does it work that?
The other option I see is creating a new PR... |
Yep, thats what I did. |
sounds good, let's create a new PR, sorry for that. |
PR created here: #50 |
Closing this one because we already merged this #50 |
As requested in #14, this PR leverages the same library for representing EVM addresses as used in the Safe UI.
I also added support to visually represent the hashes in the same way. Feel free to tell me if you'd like to have this removed if you don't like it @josepchetrit12, @xaler5